Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Builder Efficient reverts #90

Merged
merged 57 commits into from
Sep 27, 2023
Merged

Builder Efficient reverts #90

merged 57 commits into from
Sep 27, 2023

Conversation

Wazzymandias
Copy link
Contributor

@Wazzymandias Wazzymandias commented Jul 26, 2023

📝 Summary

  • "Efficient revert feature" is an optimization that reduces burden on Ethereum state database and enables efficient way to track and revert state changes across multiple transactions

    • This is important because rolling back Ethereum state when bundles or transactions fail can be incredibly expensive
  • Add support for efficient reverts using multi-transaction snapshots, which reduce the amount of state copying on bundle reverts

  • Expose CLI flag and environment variable to toggle multi-transaction snapshots on greedy and greedy-buckets builder algorithms

  • Fix unit test errors when multi-tx-snapshot is enabled

  • This work builds on top of and heavily relies on excellent work by @dvush

  • Sample graph showing latency difference between efficient revert (multi-tx-snapshot) enabled vs baseline disabled

image

📚 References


@Wazzymandias Wazzymandias requested a review from dvush July 26, 2023 02:16
@Wazzymandias Wazzymandias marked this pull request as ready for review July 26, 2023 23:53
@Wazzymandias Wazzymandias changed the title [WIP] Efficient reverts Builder Efficient reverts Jul 28, 2023
@dvush
Copy link
Contributor

dvush commented Jul 31, 2023

1 active snapshot at a time is not enough for some of the uses that we will have.

For example, there is a case with nested bundles where you can have optional bundle there.
If tx inside nested optional bundle fails the nested bundle should be reverted but outer should not.

Other example could be allowing optional txs inside bundle to be thrown out on revert (instead of commiting them as reverted). Even though we decided on "if optional tx reverts we keep it reverted" but it may be the case that we change our opinion to "if optional tx reverts we discard it".

This may become important when we merge bundles.

API can look something like that.

// first you create snapshot
statedb.NewMultiTxSnapshot()

// you commit txs here

// if you dislike the results
statedb.RevertMultiTxSnapshot()

// if you are ok with results
statedb.CommitMultiTxSnapshot() // must call it!
  • When you call Push new snapshot is created and added to the stack of active multi tx snapshots. New snapshot will accumulate all changes.
  • When you call Revert state is reverted with the latest changes from the top level snapshot, and its popped from the list.
  • When you call Commit
    1. it pops snapshots from the stack
    2. if there is another snapshot in the stack, merge all the changes of the popped one with the new head of the stack

miner/algo_greedy.go Outdated Show resolved Hide resolved
miner/env_changes.go Outdated Show resolved Hide resolved
)

for _, tx := range bundle.OriginalBundle.Txs {
if hasBaseFee && tx.Type() == types.DynamicFeeTxType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate this part of our bundle commit code. I've added this to the first version of our builder as an inheritance from other code that worked with bundles but this is just out of place tbh. This is out of scope for this pr. We can probably do that in separate one.

Normally, for transactions these checks are done in txpool so when tx is in the miner its already sanity checked. But for some reason we do that for bundles here instead of doing that in txpool.

I did this for sbundles, they are validated in txpool. I think we should do the same thing for bundles and make code for them smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added TODO to move this to txpool, it makes sense to move the logic there and would help make this loop more compact while putting validation logic in one place

miner/env_changes.go Outdated Show resolved Hide resolved
@@ -0,0 +1,417 @@
package miner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code looks better than before.

@Wazzymandias Wazzymandias marked this pull request as ready for review August 22, 2023 01:20
Copy link
Contributor

@dvush dvush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve because I cant' check all the details:

Two things that I would suggest to check:

  1. that transient storage will work with efficient reverts when they are implemented. do we need to clean them from the statedb / object structure (like we clean dirty storage)
  2. make sure that sbundle with backruns work. We have tests for them in the repo (they test envdiff implemenation).

@Wazzymandias
Copy link
Contributor Author

  1. that transient storage will work with efficient reverts when they are implemented. do we need to clean them from the statedb / object structure (like we clean dirty storage)
  2. make sure that sbundle with backruns work. We have tests for them in the repo (they test envdiff implemenation).

Done

@Wazzymandias Wazzymandias merged commit cd74c75 into main Sep 27, 2023
3 checks passed
@Wazzymandias Wazzymandias deleted the efficient-reverts branch September 27, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants